feat(threads): graceful drain hook for in-flight work before worker shutdown#1621
Conversation
|
📖 Docs: HarperFast/documentation#569 documents the new |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a graceful shutdown drain mechanism to allow workers to complete in-flight work before exiting. It adds a new shutdownDrain component, updates thread management to support extending shutdown deadlines, and integrates these drains into the worker shutdown sequence. The feedback focuses on robustly handling empty, null, or extremely large values for the drain timeout configuration to prevent silent disabling or setTimeout overflows, clearing lingering timers in runShutdownDrains to avoid open handles in tests, and adding corresponding unit tests.
kriszyp
force-pushed
the
kris/blob-send-drain-core
branch
from
1b62a23 to
5b9b499
Compare
|
Reviewed; no blockers found. |
…hutdown Adds a per-worker shutdown-drain registry (components/shutdownDrain.ts, mirroring scopeShutdown.ts) that the worker shutdown path awaits before closeServers. The force-terminate backstops (worker self-exit in manageThreads + the main-thread worker.terminate() timer) are extended only while a registered drain reports progressing work, bounded by the new replication_blobSendDrainTimeout config (default 10m, coerced), then restored to the normal short timeout once draining completes — so an unrelated worker hang is still force-killed on the normal timeout, and the main thread caps any worker-supplied deadline at the ceiling. harper-pro registers the replication blob-send drain against this hook: fix (2) of the deploy-reload blob-divergence root cause (don't tear down an in-flight replication blob SEND mid-stream during a rolling http_worker restart). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/blob-send-drain-core
branch
from
5b9b499 to
d8a63d6
Compare
New replication config option (HarperFast/harper#1621, HarperFast/harper-pro#529): bounds how long a worker drains in-flight blob sends before shutting down during a restart, so a rolling restart doesn't interrupt a transfer in progress. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
CI note (pre-existing failures, not from this PR):
This PR's diff is 5 files (shutdown-drain hook + timers + config); neither failing suite touches that path. (comment generated by Claude/KrAIs) |
…core # Conflicts: # server/threads/manageThreads.js
kriszyp
requested review from
Devin-Holland
and removed request for
kylebernhardy
Patch cherry-pick: conflictCherry-pick onto The conflict markers are committed on branch |
Devin-Holland
left a comment
Devin-Holland
left a comment
There was a problem hiding this comment.
Adds a per-worker graceful drain-before-shutdown hook registry so in-flight work (notably a replication blob send streaming to a peer) can reach a safe point before a worker restart, rather than being torn down mid-stream. Careful and fail-safe.
- runShutdownDrains never rejects (a throwing or hanging drain is logged and abandoned), is bounded by an absolute deadline via an unref'd timer that is always cleared, and no-ops with zero drains — so it can't wedge the shutdown sequence that follows.
- The backstop extension is gated on shutdownDrainsHaveWork(), so a worker hung for an unrelated reason is still force-killed on the normal short timeout. Worker-requested deadlines are clamped to the configured ceiling and to a finite value via the pure, unit-tested boundedTerminateDelay, so a rogue/buggy message can't defer the force-kill unboundedly.
- Ordering: the SHUTDOWN handler and the drain-extension race across listeners, and armSelfExit honors selfExitDrainDeadline regardless of which lands first. Worker self-exits (1x timeout headroom) before the main thread force-terminates (2x) — correct backstop order.
- Config coercion handles the string-from-YAML case and the blank-not-zero trap (a blank value must not read as Number('')===0 and silently disable draining), clamps oversized values below the 2^31 setTimeout-overflow cliff, and lets an explicit 0 disable draining. All covered by tests.
One thing worth putting on the radar: because the draining worker keeps its listening sockets up for the drain window (before closeServers), sustained normal traffic that keeps opening connections to it could keep it "busy" and push each restart closer to the ceiling — which would make routine maintenance / rolling restarts drag under load. Whether that actually happens hinges on the harper-pro blob-send drain hook's semantics:
- If the hook snapshots the set of active sends when the drain begins and only waits on those, new connections arriving mid-drain are irrelevant — it drains what was already streaming and resolves. Non-issue.
- If it treats any active send as work, a busy peer could hold a draining worker near the cap (never beyond it — the deadline is fixed at SHUTDOWN receipt and the force-terminate backstops still fire; SO_REUSEPORT also spreads new connections across the already-accepting replacement).
Worth confirming which semantics the hook uses, so operators know whether reboot time under load is bounded by work-in-flight or by ongoing traffic. Not a blocker.
Approving.
…iling boundedTerminateDelay clamped ceilingMs to MAX_TIMER_MS, but adding baseMs headroom on top of that clamp could still push the sum back over the max setTimeout delay at the extreme end of a configured ceiling (~24.8 days), which Node silently coerces to ~1ms — firing the backstop almost immediately instead of honoring the drain. Clamp the final sum too. Mirrors the same gap in armSelfExit (server/threads/manageThreads.js), which does the equivalent arithmetic inline for the worker-side backstop. Addresses cb1kenobi's review comment on #1621.
…iling boundedTerminateDelay clamped ceilingMs to MAX_TIMER_MS, but adding baseMs headroom on top of that clamp could still push the sum back over the max setTimeout delay at the extreme end of a configured ceiling (~24.8 days), which Node silently coerces to ~1ms — firing the backstop almost immediately instead of honoring the drain. Clamp the final sum too. Mirrors the same gap in armSelfExit (server/threads/manageThreads.js), which does the equivalent arithmetic inline for the worker-side backstop. Addresses cb1kenobi's review comment on #1621.
feat(threads): graceful drain hook for in-flight work before worker shutdown
New replication config option (HarperFast/harper#1621, HarperFast/harper-pro#529): bounds how long a worker drains in-flight blob sends before shutting down during a restart, so a rolling restart doesn't interrupt a transfer in progress. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 participants
Summary
Adds a generic per-worker shutdown-drain registry (
components/shutdownDrain.ts, mirroringscopeShutdown.ts) that the worker shutdown path awaits beforecloseServers(). It's the core half of fix (2) of the deploy-reload blob-divergence root cause: harper-pro registers a drain that lets an in-flight replication blob send finish instead of being torn down mid-stream by a rollinghttp_workersrestart (which leaves the peer's copy diverged). Paired harper-pro PR bumpscoreto this commit.Core stays deliberately generic — it knows nothing about blobs. A component registers a
{ hasWork, drain }hook;runShutdownDrains(deadline)awaits them (failure-isolated, never rejects, hard-bounded by the deadline).What changed
components/shutdownDrain.ts(new):registerShutdownDrain/shutdownDrainsHaveWork/runShutdownDrains, plusgetShutdownDrainCeilingMs()(coerced config read) and the pureboundedTerminateDelay()used to clamp the terminate timer.server/threads/threadServer.js: onSHUTDOWN, run registered drains beforecloseServers(). Extend the termination backstops only when a drain reports progressing work, then restore the normal short timeout once draining completes.server/threads/manageThreads.js:extendShutdownDeadline/restoreShutdownDeadline(worker) push out / restore the self-exit backstop and postEXTEND_SHUTDOWN_DEADLINEso the main thread re-arms itsworker.terminate()timer to match — clamped at the ceiling.utility/hdbTerms.ts: newreplication_blobSendDrainTimeoutconfig (default 600000 = 10 min;0disables).Where to look / lower-confidence areas
manageThreadsmain + worker,threadServer). The self-exit extension is order-independent viaselfExitDrainDeadline(the two worker SHUTDOWN listeners race). The main-thread terminate timer is armed synchronously in the same tick as theSHUTDOWNpost, so the worker's async EXTEND reply can only arrive after it exists.threadTerminationTimeoutwindow. The main thread caps any worker-supplied deadline at the ceiling.Review notes (open items, per the cross-model review)
restartWorkers'await Promise.race(waitingToFinish)throttle now waits on a draining worker's exit, so a large progressing send can delay the next worker's restart up to the ceiling. Bounded (force-terminate still fires atceiling + 2×threadTerminationTimeout; capacity is preserved by the Multi-worker HTTP rolling restart produces ~0.6–1.2s whole-pool connection-refused gap #1417 pre-started replacement) and deliberate — surfaced so operators know a deploy can wait on in-flight sends. A follow-up could stop counting a draining worker againstmaxWorkersDown.Unit tests cover the registry, config coercion, and the
boundedTerminateDelayclamp/guard arithmetic. Cross-model reviewed (Codex + Harper-domain adjudication; the Gemini/agyleg hung out and was skipped).Generated by Claude (Opus 4.8).